Fix #730 compatibility with web profiler 4 #735

ostrolucky · 2017-11-19T09:51:04Z

Doctrine profiler does not work in Symfony 4. This issue is completely ignored, so I had to give it a try. I have extended travis matrix to test Symfony 4 and fixed all Symfony 4 test failures.

ostrolucky · 2017-11-20T01:43:51Z

composer.json

+        "symfony/web-profiler-bundle": "~2.7|~3.0|~4.0"
+    },
+    "conflict": {
+        "symfony/http-foundation": "<2.6"


db.html.twig uses request.query.getBoolean(). ParameterBag#getBoolean has been introduced with 2.6

ostrolucky · 2017-11-20T01:45:29Z

composer.json

@@ -29,7 +29,7 @@
        "symfony/console": "~2.7|~3.0|~4.0",
        "symfony/dependency-injection": "~2.7|~3.0|~4.0",
        "doctrine/dbal": "^2.5.12",
-        "jdorn/sql-formatter": "~1.1",
+        "jdorn/sql-formatter": "^1.2.16",


DoctrineExtension uses SqlFormatter::$variable_attributes, introduced with 1.2.16

ostrolucky · 2017-11-20T01:49:08Z

composer.json

@@ -39,9 +39,13 @@
        "symfony/validator": "~2.7|~3.0|~4.0",
        "symfony/property-info": "~2.8|~3.0|~4.0",
        "symfony/phpunit-bridge": "~2.7|~3.0|~4.0",
-        "twig/twig": "~1.12|~2.0",
+        "twig/twig": "~1.26|~2.0",


Increased to be able to call Twig_Environment::addRuntimeLoader in tests. Notice it's require-dev section

ostrolucky · 2017-11-20T01:57:07Z

.travis.yml


 env:
  global:
    - deps=no

 before_install:
  - composer self-update
-  - if [ "$SYMFONY_VERSION" != "" ]; then composer require --no-update symfony/symfony:${SYMFONY_VERSION}; fi;
+  - if [ "$SYMFONY_VERSION" != "" ]; then jq "(.require, .\"require-dev\")|=(with_entries(if .key|test(\"^symfony/\") then .value|=\"${SYMFONY_VERSION}\" else . end))" composer.json|ex -sc 'wq!composer.json' /dev/stdin; fi;


Old approach replaced to be able to keep paths used in tests in ../vendor/symfony/{component} format, instead of introducing switches in tests to determine if ../vendor/symfony/symfony/{component} exists. Simple regex not used to keep conflicts section without change.

much simpler: composer require symfony/lts (be careful, versions there are just 2 and 3, not 2.8 and 3.4)

Good idea, though you already said disadvantages yourself. I would like to keep ability to pick exact symfony version. Other then that, this snippet will provide good example/base for future modifications of composer.json.

mikeSimonson · 2017-11-20T23:53:51Z

DataCollector/DoctrineDataCollector.php

+        foreach ($this->data['queries'] as &$queries) {
+            foreach ($queries as &$query) {
+                // HttpKernel < 3.2 compatibility layer
+                if (!method_exists($this, 'cloneVar')) {


Shouldn't both foreach be inside that if ?

stof · 2017-11-21T17:51:09Z

.travis.yml

-    - php: 5.6
+      env: SYMFONY_VERSION="2.8.*"
+    - php: 7.0
+      env: SYMFONY_VERSION="3.2.*"


don't had it. 3.2 is unmaintained

Not exactly true, end of support is in January. Until then we need to make sure it works. If you notice, there are BC layers introduced in my patch and I would like them to get tested.

stof · 2017-11-21T17:52:02Z

.travis.yml

+    - php: 7.1
+      env: SYMFONY_VERSION="3.4.*@beta"
+    - php: 7.2
+      env: SYMFONY_VERSION="4.0.*@beta"


-1 for that. This is already covered by the deps=dev job. I'm only adding version-specific jobs for LTS versions (I don't want to update this config file every 6 months to change the versions, too painful)

Which deps=dev? The one for PHP 5.6, which didn't allow to install Symfony 4? The one which was allowed to fail anyway? We can't afford for this bundle to stay incompatible with next Symfony version and figure it out only at RC phase again. These constraints should be set in the way it's clear that something is wrong when tests fail, but on the other side aren't supposed to be expected to fail randomly, which would be the case with dev dependencies. I suggest to have matrix for latest dev=alpha then. That's when failures are starting to be unwanted. But either case, if this line is removed, it means CI won't test against Symfony 4 once Symfony 4.1 is released. I think we should test against each supported minor version though. Thoughts?

stof · 2017-11-21T17:53:58Z

DependencyInjection/Compiler/EntityListenerPass.php

@@ -47,6 +47,7 @@ public function process(ContainerBuilder $container)
                }

                $resolver = $container->findDefinition($resolverId);
+                $resolver->setPublic(true);


would be better to use a ServiceLocator (by changing the lazy-loading listener resolver) rather than forcing to use public services

Not sure what do you mean, although I don't want to change behaviour. Tests expect this service to be public, meaning having it directly accessible from container. Making it otherwise would be BC break. I have done this change only to fix tests. I think it was concretely this one

DoctrineBundle/Tests/DependencyInjection/AbstractDoctrineExtensionTest.php

Line 911 in eec3981

$this->assertTrue($container->getDefinition('doctrine.orm.em1_entity_listener_resolver')->isPublic());

stof · 2017-11-21T17:54:37Z

Resources/views/Collector/db.html.twig

+        {% endfor %}
+        </tbody>
+    </table>
+{% endmacro %}


please add the final EOL

stof · 2017-11-21T17:56:14Z

Tests/ProfilerTest.php

+        $output = str_replace(["\e[37m", "\e[0m", "\e[32;1m", "\e[34;1m"], "", $output);
+        $this->assertContains("SELECT * FROM foo WHERE bar IN ('foo', 'bar');", $output);
+    }
+}


missing final EOL

stof · 2017-11-21T17:56:31Z

Tests/ProfilerTest.php

+use Symfony\Component\HttpKernel\Profiler\Profile;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+
+class ProfilerTest extends \PHPUnit\Framework\TestCase


please add a use statement

weaverryan · 2017-11-23T12:17:10Z

Hey guys! Do we need any more changes on this? I'm tracking to make sure we get a release and tag by next week.

ostrolucky · 2017-11-23T12:26:35Z

It's finished from my POV

ostrolucky · 2017-11-24T00:29:08Z

Let's please try to do the merge during this Symfony RC phase, so analogous patch for Doctrine bridge I will create can make it into 3.4 which will allow us to drop this workaround https://github.com/ostrolucky/DoctrineBundle/blob/b1e881aacb1af71293ceca13da852c01d9fc1afe/DataCollector/DoctrineDataCollector.php#L140-L149 when Symfony 2.* is EOL. If we won't make it, that workaround might have to stay there during whole life cycle of Symfony 3.4.

weaverryan · 2017-11-24T00:59:22Z

@ostrolucky Unless it's a pretty serious/obvious bug fix, nothing at this point will make it into 3.4. So, I think that workaround will need to stay. But, no big deal.

But yes, since this is broken in Symfony 4, I'd love to see a quick merge + tag so that (if there are any more issues), we can find them.

weaverryan · 2017-11-24T19:00:46Z

Thanks @mikeSimonson! I already see a new 1.8.0 tag... but I think this isn't included in it? Is that correct? Could we create a 1.8.1 with this fix?

mikeSimonson · 2017-11-25T09:15:28Z

@weaverryan done

ostrolucky added 3 commits November 19, 2017 10:48

Fix doctrine#730 compatibility with web profiler 4

6c8a34d

Fix broken phpunit 4

f5821f1

add test

dcfd560

ostrolucky force-pushed the feature-730 branch 2 times, most recently from 33849d5 to d77fb48 Compare November 19, 2017 17:49

Full Symfony 4 compatibility (according to tests) 🎉

eec3981

ostrolucky force-pushed the feature-730 branch 6 times, most recently from a58951d to b0fa88d Compare November 20, 2017 01:42

ostrolucky commented Nov 20, 2017

View reviewed changes

Test SF 3.2 + 2.8, PHP 7.2

370aaa1

ostrolucky force-pushed the feature-730 branch from b0fa88d to 370aaa1 Compare November 20, 2017 02:15

mikeSimonson reviewed Nov 20, 2017

View reviewed changes

mikeSimonson requested a review from stof November 20, 2017 23:57

More effective clonevar compatibility layer, test SF 3.4

0fa5dd3

stof requested changes Nov 21, 2017

View reviewed changes

Formatting changes, introduce stability env flag in travis matrix

b1e881a

ostrolucky force-pushed the feature-730 branch from 65c663b to b1e881a Compare November 22, 2017 00:57

mikeSimonson merged commit eb6e4fb into doctrine:master Nov 24, 2017

mikeSimonson self-assigned this Nov 24, 2017

mikeSimonson added this to the 1.7.1 milestone Nov 24, 2017

ogizanagi mentioned this pull request Nov 24, 2017

Updated the project to Symfony 4.0 RC2 symfony/demo#706

Closed

mikeSimonson modified the milestones: 1.7.1, 1.8.1 Nov 25, 2017

Athorcis mentioned this pull request Jun 2, 2018

fix compatiblity with symfony 4 VincentChalnot/DoctrineDebugBundle#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #730 compatibility with web profiler 4 #735

Fix #730 compatibility with web profiler 4 #735

ostrolucky commented Nov 19, 2017 •

edited

Loading

ostrolucky Nov 20, 2017

ostrolucky Nov 20, 2017 •

edited

Loading

ostrolucky Nov 20, 2017

ostrolucky Nov 20, 2017

stof Nov 21, 2017

ostrolucky Nov 21, 2017 •

edited

Loading

mikeSimonson Nov 20, 2017

ostrolucky Nov 21, 2017

stof Nov 21, 2017

ostrolucky Nov 21, 2017

stof Nov 21, 2017

ostrolucky Nov 22, 2017 •

edited

Loading

stof Nov 21, 2017

ostrolucky Nov 22, 2017 •

edited

Loading

stof Nov 21, 2017

stof Nov 21, 2017

stof Nov 21, 2017

weaverryan commented Nov 23, 2017

ostrolucky commented Nov 23, 2017

ostrolucky commented Nov 24, 2017 •

edited

Loading

weaverryan commented Nov 24, 2017

weaverryan commented Nov 24, 2017

mikeSimonson commented Nov 25, 2017

Fix #730 compatibility with web profiler 4 #735

Fix #730 compatibility with web profiler 4 #735

Conversation

ostrolucky commented Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

ostrolucky Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostrolucky Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Nov 23, 2017

ostrolucky commented Nov 23, 2017

ostrolucky commented Nov 24, 2017 • edited Loading

weaverryan commented Nov 24, 2017

weaverryan commented Nov 24, 2017

mikeSimonson commented Nov 25, 2017

ostrolucky commented Nov 19, 2017 •

edited

Loading

ostrolucky Nov 20, 2017 •

edited

Loading

ostrolucky Nov 21, 2017 •

edited

Loading

ostrolucky Nov 22, 2017 •

edited

Loading

ostrolucky Nov 22, 2017 •

edited

Loading

ostrolucky commented Nov 24, 2017 •

edited

Loading